PHPLIB-1323 Implement `unlink` for GridFS stream wrapper #1206

GromNaN · 2023-12-15T21:56:41Z

This is not optimized as it creates a WritableStream instance that is not really used.
The context resolver mechanism introduces in #1138 doesn't give direct access to the CollectionWrapper which is required for this feature.

alcaeus · 2023-12-18T13:18:01Z

src/GridFS/WritableStream.php

+        foreach ($revisions as $file) {
+            $this->collectionWrapper->findFileById($file->get('_id'));
+            $this->collectionWrapper->deleteFileAndChunksById($file->get('_id'));
+            $count++;
+        }


I'm wondering if we can improve on this by collecting all IDs, then deleting all files with the given filename, followed by removing all chunks for the IDs we previously found.

While there's always a chance to end up in an undesirable state, e.g. by having older revisions of files remaining, the suggestion above would reduce this by instructing the server to delete all file documents with a given name, ensuring that future requests for such files would not produce any result. Thus, if there was an error while deleting any chunks, we wouldn't end with older revisions left over.

I will implement a solution consistent with rename #1207 (comment)

This would definitely benefit from some bulk strategy to remove the fs.files documents first (likely using $in on multiple _id values), and then cleaning up the orphan chunks. I think it'd also benefit from being addressed in the GridFS spec, since we'd probably want to have a consistent way to address this in any driver that wishes to do so. And codifying that in a spec would ensure we're not overlooking some edge case.

I implemented the bulk delete

get all file ids by filename

delete all files by id

delete all chunks by file ids

The limitation is if there is more revisions that supported by a single command body.

GromNaN · 2024-01-09T12:31:20Z

src/GridFS/CollectionWrapper.php

+        $count = $this->filesCollection->deleteMany(['_id' => ['$in' => $ids]])->getDeletedCount();
+        $this->chunksCollection->deleteMany(['files_id' => ['$in' => $ids]]);
+
+        return $count ?? 0;


This is zero in case of WriteConcern=0 or not file to update.

GromNaN · 2024-01-09T12:32:33Z

src/GridFS/WritableStream.php

+        foreach ($revisions as $file) {
+            $this->collectionWrapper->findFileById($file->get('_id'));
+            $this->collectionWrapper->deleteFileAndChunksById($file->get('_id'));
+            $count++;
+        }


I implemented the bulk delete

get all file ids by filename

delete all files by id

delete all chunks by file ids

The limitation is if there is more revisions that supported by a single command body.

docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt

jmikola · 2024-01-09T15:45:36Z

src/GridFS/WritableStream.php

+    public function delete(): int
+    {
+        try {
+            return $this->collectionWrapper->deleteFileAndChunksByFilename($this->file['filename']);


I realize this is an internal class, so we're not actually introducing a public API here, but a delete() method that affects multiple revisions seems odd, which is otherwise an abstraction for writing a single revision.

In the PR description, you said:

The context resolver mechanism introduced in #1138 doesn't give direct access to the CollectionWrapper which is required for this feature.

If we look at stream_open, it accesses a CollectionWrapper after resolving the path to an array of context options. Couldn't the same approach be used here to access a CollectionWrapper directly?

I felt deep down that using ReadableStream and WritableStream wasn't correct. I refactored by adding a private getContext method to use CollectionWrapper directly for rename and unlink.

src/GridFS/CollectionWrapper.php

jmikola · 2024-01-09T18:46:00Z

src/GridFS/StreamWrapper.php

@@ -324,6 +295,14 @@ public function stream_write(string $data): int
        return $this->stream->writeBytes($data);
    }

+    public function unlink(string $path): bool
+    {
+        $context = $this->getContext($path, 'r');


Bucket::resolveStreamContext() executes findFileByFilenameAndRevision() for read modes in order to fetch the file document. Would it make sense to use 'w' here and avoid the unnecessary query?

I understand rename() needs to use 'r' since it does read the filename from the file document; however, there may be a refactoring opportunity there if you'd just like to trust the $fromPath string. I suppose it's a matter of intentionally NOP-ing on a FileNotFoundException or executing an update query that may not match anything -- so I'd be find leaving rename() as-is.

The idea of using r mode was to check that the file exists before doing operations, to throw a FileNotFoundException. This could be optimized by checking the number of updated documents after the update, but that's not feasible if Write Concern = 0 (edge case).

I reverted to throw a FileNotFoundException in rename and unlink, instead of silently returning false. This behavior is consistent with S3 stream wrapper.

GromNaN · 2024-01-10T15:58:43Z

docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt

@@ -57,6 +58,9 @@ revision is read (revision ``-1``).
 In write mode, the stream context can contain the option ``gridfs['chunkSizeBytes']``.
 If omitted, the defaults are inherited from the ``Bucket`` instance option.

+The functions `rename` and `unlink` will rename or remove all revisions of a
+filename. If the filename does not exist, these functions throw a ``FileNotFoundException``.


@jmikola removing or renaming a single revision is not supported. Even if the option gridfs[revision] is set.

jmikola · 2024-01-10T21:19:38Z

src/GridFS/StreamWrapper.php

+        $context = $this->getContext($fromPath, 'w');
+
+        $newFilename = explode('/', $toPath, 4)[3] ?? '';
+        $count = $context['collectionWrapper']->updateFilenameForFilename($context['filename'], $newFilename);


Edge case: if $fromPath and $toPath are identical, the file exists but you'll throw FileNotFoundException. That may warrant reverting to an "r" mode when selecting the context to throw eagerly.

Alternatively, you could have updateFilenameForFilename() return the matched count and assume that the update was successful or a NOP. The only normal scenario where the matched and modified counts wouldn't be equals is if the paths were the same, in which case you could probably NOP earlier and avoid calling updateFilenameForFilename() altogether (FWIW, PHP's rename() returns true in this case, but I dunno if there's some internal NOP behavior).

Good catch. I updated the code to return the matched count, and added a test for renaming with the same filename (existing and not existing).

jmikola · 2024-01-10T21:23:04Z

src/GridFS/StreamWrapper.php

@@ -338,6 +330,42 @@ public function url_stat(string $path, int $flags)
        return $this->stream_stat();
    }

+    /** @psalm-return ($mode == 'r' ? array{collectionWrapper: CollectionWrapper, file: object} : array{collectionWrapper: CollectionWrapper, filename: string, options: array}) */


Not sure if this is relevant, but the ternary here only checks for "r" and stream_open() may call this with "rb". Can you invoke functions here, or are you restricted on syntax here?

The logical or works. I also added the @return annotation so PHPStorm understands it.

* master: PHPLIB-1323 Implement `unlink` for GridFS stream wrapper (mongodb#1206) PHPLIB-1330: Sync tests for failCommand errorLabels reqs (mongodb#1214) PHPLIB-1246: Test PHP 8.3 on Evergreen (mongodb#1213) PHPLIB-1324 Implement `rename` for GridFS stream wrapper (mongodb#1207) PHPLIB-1248 Add examples on GridFS (mongodb#1196) Deprecate setting GridFS disableMD5 to false explicitly (mongodb#1205) PHPLIB-1326: Use more permissive top-level runOnRequirements (mongodb#1210) PHPLIB-1206 Add bucket alises for context resolver using GridFS StreamWrapper (mongodb#1138) Bump actions/upload-artifact from 3 to 4 (mongodb#1208) PHPLIB-1275: Replace apiargs usage in docs with extracts (mongodb#1203) Fix title formatting in Client::removeSubscriber() docs (mongodb#1204) PHPLIB-1304: Pull mongohouse image from ECR repo (mongodb#1202) Fix evergreen failures (mongodb#1200) Enable workflows to run for GitHub Merge Queue (mongodb#1199) PHPLIB-1313 Ensure the GridFS stream is saved when the script ends (mongodb#1197) PHPLIB-1309 Add addSubscriber/removeSubscriber to Client class to ease configuration (mongodb#1195) Master is now 1.18-dev

GromNaN force-pushed the PHPLIB-1323 branch 2 times, most recently from 0a08f8e to 7cbf64e Compare December 18, 2023 10:03

GromNaN marked this pull request as ready for review December 18, 2023 10:03

GromNaN requested a review from jmikola December 18, 2023 10:03

alcaeus reviewed Dec 18, 2023

View reviewed changes

GromNaN force-pushed the PHPLIB-1323 branch 2 times, most recently from cae12ee to 03f4ea2 Compare January 9, 2024 10:04

GromNaN added 2 commits January 9, 2024 13:29

PHPLIB-1323 Implement unlink for GridFS stream wrapper

abc4bca

Delete all file revisions by bulk

6da793c

GromNaN force-pushed the PHPLIB-1323 branch from 03f4ea2 to 6da793c Compare January 9, 2024 12:29

GromNaN commented Jan 9, 2024

View reviewed changes

Fix method order

fc32d0a

jmikola reviewed Jan 9, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1323 branch 2 times, most recently from 2d9c795 to 1d8e3c4 Compare January 9, 2024 16:47

GromNaN requested a review from jmikola January 9, 2024 17:07

Refactor rename and unlink to use the context directly

f8231a7

GromNaN force-pushed the PHPLIB-1323 branch from 1d8e3c4 to f8231a7 Compare January 9, 2024 17:08

jmikola reviewed Jan 9, 2024

View reviewed changes

GromNaN added 3 commits January 10, 2024 11:50

Fix phpdoc

c2bcab1

Remove exception from rename and unlink. Return false

981ede2

Thrown a FileNotFoundException in rename and unlink

70c71e8

GromNaN force-pushed the PHPLIB-1323 branch from 9f582b7 to 70c71e8 Compare January 10, 2024 15:57

GromNaN commented Jan 10, 2024

View reviewed changes

jmikola reviewed Jan 10, 2024

View reviewed changes

Fix rename with same file name

1fa8976

GromNaN force-pushed the PHPLIB-1323 branch from 01663eb to 1fa8976 Compare January 11, 2024 09:10

GromNaN requested a review from jmikola January 11, 2024 09:11

jmikola approved these changes Jan 11, 2024

View reviewed changes

GromNaN merged commit b700b10 into mongodb:master Jan 12, 2024

GromNaN deleted the PHPLIB-1323 branch January 12, 2024 12:37

GromNaN mentioned this pull request Oct 29, 2024

PHPLIB-1568 Add GridFS\Bucket::deleteByName(filename) and renameByName(filename, newFilename) #1504

Merged

PHPLIB-1323 Implement unlink for GridFS stream wrapper #1206

PHPLIB-1323 Implement unlink for GridFS stream wrapper #1206

Uh oh!

Conversation

GromNaN commented Dec 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PHPLIB-1323 Implement `unlink` for GridFS stream wrapper #1206

PHPLIB-1323 Implement `unlink` for GridFS stream wrapper #1206

GromNaN Jan 10, 2024 •

edited

Loading